Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: eliminate code duplication on conversation types #941

Merged
merged 6 commits into from
Mar 4, 2025
Merged

refactor: eliminate code duplication on conversation types #941

merged 6 commits into from
Mar 4, 2025

Conversation

SimonThormeyer
Copy link
Contributor

@SimonThormeyer SimonThormeyer commented Feb 28, 2025 •
edited
Loading

What's new in this PR

See title and commit history

PR Submission Checklist for internal contributors
  • The PR Title
    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 28, 2025 •
edited
Loading

🐰 Bencher Report

Branchsimon/refactor/eliminate-code-duplication-on-conversation-types
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		19.71 ms
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6.90 ms
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		9.58 ms
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		12.65 ms
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		15.08 ms
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		17.27 ms
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold		982.17 ms
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold		6.86 ms
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold		84.56 ms
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold		219.91 ms
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold		424.60 ms
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold		673.21 ms
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		117.55 ms
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		28.68 ms
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		46.12 ms
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		61.35 ms
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		79.68 ms
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		95.08 ms
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold		19.24 ms
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold		116.63 ms
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold		36.22 ms
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold		56.81 ms
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold		76.73 ms
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold		95.81 ms
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		28.02 ms
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6.88 ms
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		9.25 ms
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		12.14 ms
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		18.34 ms
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		22.09 ms
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold		30.94 ms
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold		137.36 ms
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold		115.57 ms
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold		94.49 ms
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold		72.34 ms
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold		51.30 ms
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		137.53 ms
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		7.07 ms
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		34.03 ms
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		59.98 ms
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		89.12 ms
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		111.63 ms
🐰 View full continuous benchmarking report in Bencher

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.93939% with 34 lines in your changes missing coverage. Please review.

Project coverage is 79.76%. Comparing base (411e8d9) to head (2dac716).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crypto/src/mls/conversation/mod.rs 97.17% 14 Missing ⚠️
crypto-ffi/src/generic/mod.rs 0.00% 8 Missing ⚠️
...src/mls/conversation/immutable_conversation/mod.rs 0.00% 6 Missing ⚠️
crypto/src/mls/mod.rs 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #941      +/-   ##
==========================================
+ Coverage   79.54%   79.76%   +0.22%     
==========================================
  Files         118      114       -4     
  Lines       21089    21056      -33     
==========================================
+ Hits        16775    16796      +21     
+ Misses       4314     4260      -54
Files with missing lines Coverage Δ
crypto-ffi/src/generic/context/e2ei.rs 0.00% <ø> (ø)
crypto-ffi/src/generic/context/mod.rs 0.00% <ø> (ø)
crypto/src/context.rs 72.09% <100.00%> (+2.86%) ⬆️
crypto/src/e2e_identity/conversation_state.rs 82.01% <ø> (ø)
crypto/src/e2e_identity/mod.rs 98.92% <ø> (ø)
crypto/src/e2e_identity/rotate.rs 99.22% <ø> (ø)
crypto/src/mls/buffer_external_commit.rs 98.47% <ø> (ø)
crypto/src/mls/conversation/buffer_messages.rs 98.45% <ø> (ø)
.../src/mls/conversation/conversation_guard/commit.rs 99.11% <100.00%> (+0.26%) ⬆️
...src/mls/conversation/conversation_guard/encrypt.rs 97.59% <ø> (ø)
... and 12 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 411e8d9...2dac716. Read the comment docs.

@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/conversation-guard-e2ei-WPB-15589-b branch 3 times, most recently from 2c59de7 to 411e8d9 Compare March 3, 2025 08:59
Base automatically changed from simon/refactor/conversation-guard-e2ei-WPB-15589-b to main March 3, 2025 10:24
@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch 3 times, most recently from d0a28df to 7f35bf9 Compare March 3, 2025 11:07
@SimonThormeyer SimonThormeyer changed the title [Don't merge] Simon/refactor/eliminate code duplication on conversation types refactor: eliminate code duplication on conversation types Mar 3, 2025
@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch from 7f35bf9 to 610da70 Compare March 3, 2025 11:10
@SimonThormeyer SimonThormeyer marked this pull request as ready for review March 3, 2025 11:11
@SimonThormeyer SimonThormeyer requested a review from a team as a code owner March 3, 2025 11:11
@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch 4 times, most recently from 7a5a503 to d4cd087 Compare March 3, 2025 11:52
coriolinus
coriolinus reviewed Mar 3, 2025
View reviewed changes
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great so far; I love the fundamental idea here.

@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch 4 times, most recently from 6b19d53 to 9cc6b4e Compare March 3, 2025 15:36
@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch 2 times, most recently from 821f42b to 2dac716 Compare March 3, 2025 15:46
coriolinus
coriolinus approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work!

@SimonThormeyer
refactor: add and implement HasClientAndProvider trait
d52d55d 
New trait is added and implemented:
* `HasClientAndProvider`, implemented by `CentralContext`
and `MlsCentral`
@SimonThormeyer
refactor: add and implement Conversation traits
8b21111 
Two traits are added:
* `ConversationWithMls` an internal trait, representing a
`Conversation` with an associated type implementing the
`HasClientAndProvider` trait
* `Conversation`, the public trait that exposes the read-only API of a
conversation. This contains all the shared code between
`ImmutableConversation` and `ConversationGuard`.
@SimonThormeyer
refactor: implement trait and remove duplication on ConversationGuard
0bb0f61
@SimonThormeyer
refactor: implement trait and remove duplication on `ImmutableConvers…
e993896 
…ation`
@SimonThormeyer
refactor: move WireIdentity tests to conversation module
f63bf3c 
After removing duplicated code, we drop the `e2ei` module inside
the conversation guard module. Here is the new place for the tests that
were in that module.
@SimonThormeyer
refactor: fix usage sites of types implementing Conversation
516f701 
I.e., import the new trait.
@SimonThormeyer SimonThormeyer force-pushed the simon/refactor/eliminate-code-duplication-on-conversation-types branch from 2dac716 to 516f701 Compare March 4, 2025 08:22
@SimonThormeyer SimonThormeyer merged commit 516f701 into main Mar 4, 2025
@SimonThormeyer SimonThormeyer deleted the simon/refactor/eliminate-code-duplication-on-conversation-types branch March 4, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coriolinus coriolinus coriolinus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SimonThormeyer @codecov-commenter @coriolinus